REF: Use descriptors for properties #483
Conversation
| clear_if_none = not required | ||
| self.clear_if_none = clear_if_none | ||
|
|
||
| def __get__(self, instance: HasProperties[U], owner: Any = None) -> Optional[U]: |
There was a problem hiding this comment.
LMK if the type signatures are confusing (I think they are).
But the usage below will hopefully clear things up. U is the type of the actual object being returned, like str for description, or List[float] for extent.
I see now that we can (maybe) tighten this up a bit. We know that if the property is required then return type is just U rather than Optional[U]. I'll see if mypy can handle that.
|
Fixed that CI failure (apparently I forgot to re-run the test after tweaking it). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #483 +/- ##
==========================================
+ Coverage 91.66% 96.25% +4.58%
==========================================
Files 40 71 +31
Lines 5245 9902 +4657
==========================================
+ Hits 4808 9531 +4723
+ Misses 437 371 -66 ☔ View full report in Codecov by Sentry. |
|
Thanks. Fixed in 476b20f hopefully. |
There was a problem hiding this comment.
Thanks for putting this together @TomAugspurger, it's really helpful to see it in practice.
My initial impression is that this seems to add a lot of complexity to how these properties are handled and I'm not sure that the reduction in boilerplate code is worth it, especially we have to implement separate descriptor classes for each extension. If there is a way to make this abstract enough to work across all extensions, however, then I think the trade-off might be worth it.
The other concern is the typing. Having the attributes typed as something like _Property[str] instead of str is not ideal, since these types are used in the auto-generated API docs. I did a little experimenting to see if there is a way to have the type annotations reflect the return type of _Property.__get__, but came up empty.
Yeah, I think that's doable. Glancing at
Yeah, the docs don't look great.
At the very least, the docstring can be set, so that I'm less sure about the type annotations showing up there. As you say, the (FYI, I consider this pretty low priority. I'll hold on any more experimentation / expanding this to other extensions until we have a consensus on whether it's worth proceeding, which can wait till after 1.0) |
|
I think the complexity of dealing with docstrings and internal code outweighs the usefulness of removing boilerplate. I'm in favor of closing - who else has thoughts? |
|
I'm in favor of closing as well. I made another attempt at getting this to work while working on using dictionaries to back all of the STAC Items and the typing is really the big hindrance. I agree I don't think the trade-off is worth it. |
|
SGTM. I'll keep this branch around for future reference. Just a note on the docstring, that's not too hard to fix. We could pass the docstring to the descriptor description: _Property[Optional[str]] = _Property(DIM_DESC_PROP, help="A detailed description...")and have the class set My main concern is around consistency in the behavior of these getters and setters across extensions (and core pystac objects). This could partially be resolved by refactoring properties like @extent.setter
def extent(self, v: Optional[List[Optional[float]]]) -> None:
if v is None:
self.properties.pop(DIM_EXTENT_PROP, None)
else:
self.properties[DIM_EXTENT_PROP] = vto call a |
|
https://adamj.eu/tech/2021/10/18/python-type-hints-how-to-type-a-descriptor/ might be helpful for the typing. It's been a while since I read through it. |
Related Issue(s): #
#367
Description:
A proof of concept for using a descriptor to reduce the boilerplate for defining getters and setters for the attributes on an extension. This cuts down on the lines of code, and should help keep the behavior of getters and setters consistent, since they're using the same implementation.
If we're happy with this, we can expand it to other places in the library. There will be some tradeoff between the complexity of the
__get__and__set__methods and how widely it can be used. Right now it's tailored to theDatacubeextension.PR Checklist:
pre-commit run --all-files)scripts/test)